Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ColorManagement: renamed .legacyMode to .enabled #24700

Closed
wants to merge 1 commit into from

Conversation

WestLangley
Copy link
Collaborator

The current API is as follows:

THREE.ColorManagement.legacyMode = false; // false means enabled

This PR changes the API to:

THREE.ColorManagement.enabled = true;

@donmccurdy
Copy link
Collaborator

@gkjohnson and I had discussed this as well...

The semantic issue is that color management (as a concept, not THREE.ColorManagement) includes more than just handling input colors, which is all the flag currently does. "Disabling color management" does not disable renderer output encoding, for example.

Maybe that is not confusing though? If you think of it not as a toggle on all color management, but as a toggle on the things that THREE.ColorManagement does.

I am happy with the change. 👍

@donmccurdy
Copy link
Collaborator

We should accept .legacyMode=false as a fallback for the next ?? releases though. This option is enabled by default in R3F, and would otherwise cause some real churn.

/cc @drcmda FYI

@CodyJasonBennett
Copy link
Contributor

No worries. We can work around it fine there.

@gkjohnson
Copy link
Collaborator

The semantic issue is that color management (as a concept, not THREE.ColorManagement) includes more than just handling input colors, which is all the flag currently does. "Disabling color management" does not disable renderer output encoding, for example.

If everyone wants to change this I won't strongly fight it but I think it's worth making sure the whole picture is known. Currently setting "legacyMode = true" (what is being suggested to be changed to "ColorManagement.enabled = false") is not truly unmanaged color - it has just been mistakenly referred to as such. So to that end I would consider it incorrect to change the current nomenclature to "enabled".

At some point what we're calling "legacy mode" should be removed from the project. And it should be decided if we really plan to support actual unmanaged color workflows which would basically just mean ignoring all "encoding" fields throughout the renderer. I still don't see a lot of value in that. And with the recent change to use SRGB conversion on texture upload with the WebGL API the question of performance implication is less relevant, I think.

@drcmda
Copy link
Contributor

drcmda commented Sep 27, 2022

the most important for us is that color management is switched on. like @donmccurdy mentioned, we have this on for a longer while and it works great! but since it's opt in atm there are unavoidable bugs that are only fixed if it's the default.

the amount of churn is still a burden, since we have to take four three different three breaking changes into account:

  • old three w/o color management, needs .convertSRGBToLinear hacks
  • new three with CM.legacy = false
  • newer three with CM.legacyMode = false
  • even never three with CM.enabled = true

but seeing the = true, and in hopes this won't change again, perhaps it's worth it. though i wish it had semver, what a struggle.


if in the end, for whatever reason, it is not changed to "true" then please let us not change the name, because that would create churn for nothing.

@LeviPesin
Copy link
Contributor

new three with CM.legacy = false

What version is that?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Sep 27, 2022

we have this on for a longer while and it works great! but since it's opt in atm there are unavoidable bugs that are only fixed if it's the default.

To clarify for others, "unavoidable bugs" refers to the fact that R3F cannot always execute CM.legacyMode = false before its users have begun constructor colors. Consider:

// root.js
import { ... } from './a.js';
import { ... } from './b.js';
// a.js
import { Color } from 'three';
const DEFAULT_COLOR = new THREE.Color(0x404040); // CM.legacyMode is still true
// b.js
import { ... } from '@react-three/fiber'; // sets CM.legacyMode to false

So the CM.legacyMode property works for a user who sets it at the root of their application, but a library like R3F can only "opt in" on behalf of its users with these limitations, and sometimes users get bitten. Here's an example shared in three.js Slack earlier today:

https://codesandbox.io/s/red-grass-60xi6p


@drcmda I think that @WestLangley is hoping to improve the API, this does not have to be a breaking change for R3F. As proposed above, we could continue accepting legacyMode=false as an undocumented alias until R3F (eventually) increases its minimum three.js version to include the update.

None of our official examples use CM.legacyMode = false today. I suspect it will be a hard sell to turn it on by default until that happens, and we have a chance to identify problems. Many of the changes we'd like to make to color-related APIs are interlinked (see #23614) and to the extent that we can unlink them — e.g. changing a property name by itself — without adding breaking changes, this will go faster.


At some point what we're calling "legacy mode" should be removed from the project.

By "legacy mode" we are talking about ignoring the colorSpace arguments to THREE.Color constructors and setters, correct? So that legacyMode=false means hexadecimal and CSS-style inputs are understood to be sRGB rather than Linear-sRGB?

I'd be glad to see legacyMode=false (or similar) enabled by default, but I worry that I may have stepped on toes getting THREE.ColorManagement added in the first place. So I am hoping to wait and let others try it out and guide what happens to it next. 😇

And it should be decided if we really plan to support actual unmanaged color workflows which would basically just mean ignoring all "encoding" fields throughout the renderer. I still don't see a lot of value in that.

Nope, let's not do that! 🙅🏻‍♂️

I'm still unsure of what to recommended for unlit work, but ignoring .encoding fields does not feel like the right solution.

@WestLangley WestLangley added this to the r146 milestone Sep 30, 2022
@WestLangley
Copy link
Collaborator Author

OK. Waited until after the release to add the milestone. :-)

@LeviPesin
Copy link
Contributor

I think it is better to keep the name as it is? Per @gkjohnson's reasons.

@WestLangley WestLangley removed this from the r146 milestone Oct 5, 2022
@WestLangley
Copy link
Collaborator Author

Closing, then...

@WestLangley WestLangley closed this Oct 5, 2022
@WestLangley WestLangley deleted the dev-color_mgmt branch October 5, 2022 23:19
@donmccurdy
Copy link
Collaborator

Would you feel more comfortable using ColorManagement.enabled = true in examples, if it were renamed as proposed here? I still feel that THREE.ColorManagement is a good direction, e.g. as it simplifies things like #24362 (comment). I do not care very much what it is named, and would be happy to support any change you prefer.

@WestLangley
Copy link
Collaborator Author

Yes, I would prefer merging this PR.

@donmccurdy
Copy link
Collaborator

Ok! Continued in #24940.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants